refactor readers to use type parameters and not concrete vtables #207

cosmicexplorer · 2024-07-06T07:24:12Z

Problem

As described in gyscos/zstd-rs#288 (comment), our usage of &'a mut dyn Read in ZipFileReader stops us from being able to impl Send or Sync, or generally to send a ZipFile into a separate thread than the one owning the ZipArchive it belongs to. While this is generally not an issue, as the single synchronous Read + Seek handle makes it difficult to farm out work to subthreads, for the purposes of #72, we would like to be able to reuse some code instead of creating an entirely separate implementation.

That brings us to the second set of issues, surrounding zipcrypto support:

all of our readers go through the CryptoReader enum and the make_crypto_reader() method, even though zip crypto is a very uncommon use case,
ZipFile itself contains some very complex mutable state with both a ZipFileReader and a CryptoReader, even though ZipFileReader itself also contains a CryptoReader,
Crc32Reader has to maintain a special flag to disable CRC checking in the special case of AE2 encryption, which is a sign of a leaky abstraction and may easily introduce a failure to check CRC in other cases by accident in the future.

Solution

Luckily, all of this becomes significantly easier without too many changes:

Create src/unstable/read.rs to reimplement read::ZipFile with the new unstable::read::ZipEntry.
Make EntryReader and ZipEntry with a parameterized reader type R instead of an internal &'a mut dyn Read.
Make new versions of e.g. ZipArchive::by_index() returning ZipResult<ZipEntry<'_, impl Read + '_>> to avoid leaking internal structs while retaining the ability to rearrange the layout of our internal structs.
Vastly improve readability of CryptoReader creation through CryptoVariant.

This also leads to a significantly improved refactoring of streaming support in the streaming submodule of src/unstable/read.rs.

Result

zipcrypto support doesn't muck up the non-crypto methods, ZipEntry is now Send, and src/unstable/read.rs is significantly easier to read with equivalent functionality. This will have to be a breaking change in order to achieve Send-ability, but I believe the readability/maintainability is substantially improved with this change.

src/read.rs

src/unstable/read.rs

+            let mut data = ZipFileData::from_local_block(block, reader)?;
+
+            match parse_extra_field(&mut data) {
+                /* FIXME: check for the right error type here instead of accepting any old i/o


src/unstable/read.rs

+            // We can't use the typical ::parse() method, as we follow separate code paths depending
+            // on the "magic" value (since the magic value will be from the central directory header
+            // if we've finished iterating over all the actual files).
+            /* TODO: smallvec? */


src/unstable/read.rs

+        }
+        if let Some(info) = data.aes_mode {
+            #[cfg(not(feature = "aes-crypto"))]
+            /* TODO: make this into its own EntryReadError error type! */


src/unstable/read.rs

src/crc32.rs

src/unstable/read.rs

src/crc32.rs

+            if self.check == res {
+                Ok(())
+            } else {
+                /* TODO: make this into our own Crc32Error error type! */


Cargo.toml

- also use git dep for zstd to pull in removal of R: BufRead bound - move new methods into unstable/read.rs - support getting aes verification info from a raw ZipEntry - flesh out other methods from ZipFile -> ZipEntry - now create a streaming abstraction - move some methods around

src/crc32.rs

+            return self
+                .check_matches()
+                .map(|()| 0)
+                /* TODO: use io::Error::other for MSRV >=1.74 */


src/crc32.rs

+        if self.check == res {
+            Ok(())
+        } else {
+            /* TODO: make this into our own Crc32Error error type! */


src/crc32.rs

+        if self.check == res {
+            Ok(())
+        } else {
+            /* TODO: make this into our own Crc32Error error type! */


src/crc32.rs

+        if self.check == res {
+            Ok(())
+        } else {
+            /* TODO: make this into our own Crc32Error error type! */


This reverts commit fd11f28.

This reverts commit f0faf24.

This reverts commit 3abac52.

src/crc32.rs

Pr0methean

This is a partial review.

Cargo.toml

src/crc32.rs

src/read.rs

Pr0methean · 2024-07-19T21:35:52Z

src/unstable/read.rs

@@ -0,0 +1,751 @@
+//! Alternate implementation of [`crate::read`].


Document when to use this and when to use the main impl. Also, please remove everything we're not using internally -- we don't need a second public API.

Pr0methean · 2024-07-19T21:39:49Z

src/read.rs

+use crate::unstable::read::{
+    construct_decompressing_reader, find_entry_content_range, CryptoEntryReader, CryptoVariant,
+};
+pub use crate::unstable::read::{ArchiveEntry, ZipEntry};


This doesn't need to be public.

Suggested change

pub use crate::unstable::read::{ArchiveEntry, ZipEntry};

pub(crate) use crate::unstable::read::{ArchiveEntry, ZipEntry};

src/unstable/read.rs

Pr0methean · 2024-08-02T01:45:24Z

https://github.com/zip-rs/zip2/tree/wrapped-reader-refactor is a squashed and rebased version of this PR, but it doesn't build (mainly due to type mismatches involving BufRead).

Pr0methean · 2024-08-03T00:40:36Z

Update: the branch now passes unit tests, and fuzz tests are running on it; but the new code specialized for seekable files is currently unused. @cosmicexplorer Is this a good place for you to resume work from?

cosmicexplorer · 2024-08-10T07:16:02Z

You rock!!! I have just rebased, will address your comments now.

cosmicexplorer · 2024-08-11T11:10:39Z

So @Pr0methean I'd like to raise a small discussion here: the reason I initially made this a draft PR is because I believe there are several shortcomings that necessitate a slight reimplementation, some requiring breaking API changes:

propagating Send and/or Sync bounds from R to ZipFile<R> is literally impossible with the current public API that type-erases to dyn Read.
- We could make two separate copies, one with dyn Read + Send, but that would be a lot of trouble and not worth it.
- We could just assert that R is Send and always carry a dyn Read + Send, but that would be unsound.
ZipFile has a lot of stateful logic, including a ZipFileReader::NoReader state, even necessitating an invalid_state() helper method for error checking.
- Notably (and imo inexcusably), ZipFile has a Drop impl which is only used when reading stream archives (to ensure we read out the rest of the entry from the stream), but this niche use case necessitates every reader struct in the crate to provide an .into_inner() method.
To underline the reason this Drop impl for ZipFile is a bad idea, there is currently a bug in ZipStreamReader which drops the first ZipStreamFileMetadata entry, because read_zipfile_from_stream() does not save the contents of the last ZipLocalEntryBlock it reads which has spec::Magic::CENTRAL_DIRECTORY_HEADER_SIGNATURE.
- Basically, streaming requires special handling, not being bolted onto ZipFile.
Because streaming requires special handling and separate structs for entries, we also need to be able to get access to all the methods for accessing ZipFileData that we have on ZipFile, but for other structs. That's why this change introduces EntryData, which implements all the methods traditionally provided as member functions of ZipFile.
- This means we don't need to go through the ZipStreamVisitor interface, and we can have direct control over when to iterate through each entry of the streaming zip file. That's what zip::unstable::read::streaming provides.
- Importantly, StreamingArchive maintains the internal state necessary to retain the bytes of the first metadata entry after we go through all the local file entries, fixing the bug in ZipStreamReader which drops the first metadata entry.
Finally, ZipFileReader::Compressed(Box<Crc32Reader<Decompressor<io::BufReader<CryptoReader<'a>>>>>) always goes through the logic for CryptoReader, when encrypted zip files are an extremely niche use case. We shouldn't be cluttering up our type definitions or our stack traces with encryption logic when most users are never going to touch it!
- As an illustrative example, see our poor Crc32Reader at
  
  zip2/src/crc32.rs
  
  Line 15 in 3f6768e
  
  enabled: bool,
  
  which is completely unused if we're doing AE2 decryption. We shouldn't even be instantiating a Crc32Reader at all, and this change means we actually don't, instead of having to insert little workarounds and bolt on flags to disable stuff; see https://github.com/cosmicexplorer/zip/blob/04f6ced5f2197d0ef790774442e6371ab904229e/src/read.rs#L1329-L1345:

        let final_reader = if data.encrypted {
            let crypto_variant = CryptoKeyValidationSource::from_data(data)?;
            let is_ae2_encrypted = crypto_variant.is_ae2_encrypted();
            let crypto_reader = crypto_variant.make_crypto_reader(password, content)?;
            let entry_reader =
                construct_decompressing_reader(&data.compression_method, crypto_reader)?;
            if is_ae2_encrypted {
                /* Ae2 voids crc checking: https://www.winzip.com/en/support/aes-encryption/ */
                CryptoEntryReader::Ae2Encrypted(entry_reader)
            } else {
                CryptoEntryReader::NonAe2Encrypted(NewCrc32Reader::new(entry_reader, data.crc32))
            }
        } else {
            /* Not encrypted, so do the same as in .by_index_generic(): */
            let entry_reader = construct_decompressing_reader(&data.compression_method, content)?;
            CryptoEntryReader::Unencrypted(NewCrc32Reader::new(entry_reader, data.crc32))
        };

So that's my case for the API changes. We can do them in pieces, but I'm proposing this intentionally breaking change that I believe will improve the internal architecture and correctness, increase our ability to perform optimizations, and generate more flexible entry structs in our public API.

cosmicexplorer · 2024-08-11T11:14:23Z

I'm experiencing an incredibly strange issue on all web browsers where pushing to this branch isn't being propagated to this PR, and comparing https://github.com/zip-rs/zip2/compare/master...cosmicexplorer:zip:wrapped-reader-refactor?expand=1 acts like they're the same branch. I'm going to close this and create a new PR with the above proposal and see if that fixes this weird issue.

cosmicexplorer · 2024-08-11T11:29:24Z

Moved to #233 which fixes the weird github error in this PR.

github-advanced-security bot found potential problems Jul 6, 2024

View reviewed changes

cosmicexplorer force-pushed the wrapped-reader-refactor branch from 26cf755 to 134b355 Compare July 6, 2024 15:29

This was referenced Jul 6, 2024

parallel/pipelined extraction #208

Closed

parallel operations workstream #193

Open

cosmicexplorer commented Jul 6, 2024

View reviewed changes

Cargo.toml Show resolved Hide resolved

cosmicexplorer force-pushed the wrapped-reader-refactor branch from 134b355 to 020a61a Compare July 16, 2024 19:51

cosmicexplorer added 3 commits July 16, 2024 17:25

remove read_zipfile_from_stream()

3abac52

fix up some public methods to use new impls

b463f02

remove the old ZipFile API entirely

f0faf24

cosmicexplorer force-pushed the wrapped-reader-refactor branch from 2574c06 to f0faf24 Compare July 16, 2024 21:25

cosmicexplorer added 2 commits July 16, 2024 17:37

remove a few more dyns

bf63d19

make AesModeInfo pub(crate) to make its intention clear

3960e34

github-advanced-security bot found potential problems Jul 16, 2024

View reviewed changes

cosmicexplorer added 6 commits July 16, 2024 18:25

refactor streaming

27ba4d9

fmt and clippy

9fee2c8

make ArchiveEntry more publicly accessible

c360b15

allow(private_interfaces) -> allow(warnings)

8c7cae1

fix impl trait return

fb37849

fix private bounds on stable

872bd35

cosmicexplorer force-pushed the wrapped-reader-refactor branch from 7223f67 to 872bd35 Compare July 16, 2024 23:03

github-advanced-security bot found potential problems Jul 16, 2024

View reviewed changes

src/crc32.rs Outdated

if self.check == res {

Ok(())

} else {

/* TODO: make this into our own Crc32Error error type! */

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment

remove now-dead code

fd11f28

github-advanced-security bot found potential problems Jul 16, 2024

View reviewed changes

src/crc32.rs Outdated

if self.check == res {

Ok(())

} else {

/* TODO: make this into our own Crc32Error error type! */

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment

cosmicexplorer marked this pull request as ready for review July 16, 2024 23:10

fix for windows

a28a43d

cosmicexplorer mentioned this pull request Jul 16, 2024

Initial support for tokio #121

Open

cosmicexplorer added major This >~1000 SLOC PR is likely to cause hard-to-resolve conflicts with other open PRs once merged. rust Pull requests that update Rust code labels Jul 16, 2024

Revert "remove now-dead code"

423a263

This reverts commit fd11f28.

cosmicexplorer added 3 commits July 16, 2024 20:34

Revert "remove the old ZipFile API entirely"

c83e9b6

This reverts commit f0faf24.

Revert "remove read_zipfile_from_stream()"

3259c5e

This reverts commit 3abac52.

remove unused

bbee45b

cosmicexplorer force-pushed the wrapped-reader-refactor branch from 315623d to bbee45b Compare July 17, 2024 00:45

github-advanced-security bot found potential problems Jul 17, 2024

View reviewed changes

src/crc32.rs Dismissed Show dismissed Hide dismissed

Pr0methean requested changes Jul 19, 2024

View reviewed changes

Pr0methean added the Please address review comments Some review comments are still open. label Jul 19, 2024

Pr0methean added the Please fix rebase conflicts Please rebase this PR against master and fix the conflicts. label Jul 27, 2024

Pr0methean removed the Please fix rebase conflicts Please rebase this PR against master and fix the conflicts. label Aug 3, 2024

cosmicexplorer mentioned this pull request Aug 11, 2024

New read api #233

Draft

2 tasks

cosmicexplorer closed this Aug 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor readers to use type parameters and not concrete vtables #207

refactor readers to use type parameters and not concrete vtables #207

cosmicexplorer commented Jul 6, 2024 •

edited

Loading

Pr0methean left a comment

Pr0methean Jul 19, 2024

Pr0methean Jul 19, 2024

Pr0methean commented Aug 2, 2024

Pr0methean commented Aug 3, 2024 •

edited

Loading

cosmicexplorer commented Aug 10, 2024

cosmicexplorer commented Aug 11, 2024

cosmicexplorer commented Aug 11, 2024

cosmicexplorer commented Aug 11, 2024

		@@ -0,0 +1,751 @@
		//! Alternate implementation of [`crate::read`].

	pub use crate::unstable::read::{ArchiveEntry, ZipEntry};
	pub(crate) use crate::unstable::read::{ArchiveEntry, ZipEntry};

refactor readers to use type parameters and not concrete vtables #207

refactor readers to use type parameters and not concrete vtables #207

Conversation

cosmicexplorer commented Jul 6, 2024 • edited Loading

Problem

Solution

Result

Pr0methean left a comment

Choose a reason for hiding this comment

Pr0methean Jul 19, 2024

Choose a reason for hiding this comment

Pr0methean Jul 19, 2024

Choose a reason for hiding this comment

Pr0methean commented Aug 2, 2024

Pr0methean commented Aug 3, 2024 • edited Loading

cosmicexplorer commented Aug 10, 2024

cosmicexplorer commented Aug 11, 2024

cosmicexplorer commented Aug 11, 2024

cosmicexplorer commented Aug 11, 2024

cosmicexplorer commented Jul 6, 2024 •

edited

Loading

Pr0methean commented Aug 3, 2024 •

edited

Loading